fix(vllm): derive expert-parallel world size instead of reading a missing field - #615
Open
nicolasnoble wants to merge 3 commits into
Open
Conversation
…sing field Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Contributor
WalkthroughThe change derives expert-parallel world size from ChangesExpert-parallel identity calculation
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Around line 101-117: Update the version-bump procedure in CLAUDE.md to use a
repository-wide text search that includes Markdown, YAML, Helm, and other
relevant files, rather than restricting grep to .rs, .py, and .toml. Instruct
users to classify intentional public-image references separately while updating
stale version fixtures.
- Around line 129-145: Execute the Rust source-identity assertions before
running the Python suite: add cargo test --workspace or a targeted
modelexpress-server source-identity test command, rather than relying on cargo
check --workspace --tests. Preserve the existing cross-check workflow and ensure
the Rust tests actually run.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8218b3f5-f5ec-4335-a4ca-3f6b411b7782
📒 Files selected for processing (8)
.github/workflows/modelexpress-ci-tests.ymlCLAUDE.mdci/TEST_PLAN.mdci/k8s/client/vllm/manifest-azure-ep2.yamldocs/metadata.mdmodelexpress_client/python/modelexpress/metadata/publish.pymodelexpress_client/python/tests/test_publish_source_identity.pymodelexpress_server/src/p2p/backend/memory.rs
nicolasnoble
requested review from
zhengluo-nv
and removed request for
AndyDai-nv
August 12, 2026 21:08
…just check Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
zhengluo-nv
reviewed
Aug 13, 2026
Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
vLLM's ParallelConfig has no expert_parallel_size attribute. metadata/publish.py read one anyway with a getattr default of 0, so the expert-parallel component of every vLLM mx_source_id has been a constant zero. sglang and TRT-LLM populate it for real, so this is the vLLM path only.
It produces false matches, not failed ones. An MoE deployment with --enable-expert-parallel and one without hash identically, and after #363 both have a worker at worker_rank 0, so the transfer succeeds and delivers the wrong expert weights with nothing raised and no disk fallback.
build_source_identity now computes tp * dp * pcp when enable_expert_parallel is set and 1 otherwise, following vllm/model_executor/layers/fused_moe/config.py. The attribute is prefill_context_parallel_size; the longer name in parallel_state.py is a local parameter, and reading that one would reintroduce the same silent default.
No dp field on SourceIdentity. manifest-azure-dp2.yaml and TEST_PLAN row 5.2 describe that omission as deliberate, DP cores being interchangeable copies, and the corrected product already carries dp. It does leave pcp unhashed, which fits better with the wider topology work.
Adds the first test coverage for build_source_identity, and corrects five places in CI and TEST_PLAN claiming that enabling EP raises the value above zero. None were executable assertions, which is why the ep2 job passed with both sides publishing the same wrong number.
This changes published mx_source_id values for vLLM sources, non-MoE included, without a version change.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests